Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

pack: enable kafka on windows #9108

Closed
wants to merge 1 commit into from
Closed

pack: enable kafka on windows #9108

wants to merge 1 commit into from

Conversation

prince-chrismc
Copy link

@prince-chrismc prince-chrismc commented Jul 18, 2024 •
edited
Loading

This PR enables Kafka in/out by default for fluentbit. The research I found the original reason was the CI/CD pipeline was not about to support it but in the years since it has been greatly improved and I was able to get it to build prince-chrismc#2. I used these binaries to deploy to 75 devs across several locations.

I tried to join the slack channel, but I never got the invite and slack says there was a problem. Please let me know if there's more information I can provide I was able to attend the community meeting and share this! ❤️

Related issues/pr

#8662
#7010
#5204

Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

 [SERVICE]
     flush           0.5
     grace           5
     log_Level       debug
     log_file        .fluentbit.log.txt
     Mem_Buf_Limit   100MB
 
     http_server     on
 
 [INPUT]
     Name tail 
     Path .buildscript.log.txt
     Path_Key file_name
     Read_from_head true
 
 # [INPUT]
 #     Name tail 
 #     Path .fluentbit.log.txt
 #     Path_Key file_name
 # #     Read_from_head   true
 
 [OUTPUT]
     Name        kafka
     Match       *
     Brokers     kafka01:9092,kafka02:9092,kafka03:9092
     Topics      build_logs
     timestamp_key nanoseconds
     rdkafka.log.connection.close false
     rdkafka.request.required.acks 1
     # rdkafka.linger.ms 2500
     rdkafka.linger.ms 500
     # rdkafka.debug broker,topic,msg
     rdkafka.message.max.bytes 2147483647
     rdkafka.batch.size 2147483647
     rdkafka.batch.num.messages 2500000
     rdkafka.queue.buffering.max.messages 250000
     rdkafka.queue.buffering.max.kbytes 2147483647
 
 [FILTER]
     Name record_modifier
     Match *
     Record build_number ${BUILD_ID}
     Record hostname ${HOSTNAME}
     Record username ${USERNAME}
     Record version_id ${VERSION_ID}
 
 [FILTER]
     Name          lua
     Match         *
     script        add_timestamp.lua
     call          add_timestamp
  • Example configuration file for the change
  • Debug log output from testing the change
  • Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

I was not able to run the tests local but I did run the maintainers ones in my fork to ensure it works.

  • Run local packaging test showing all targets (including any new ones) build.
  • Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@cosmo0920
Copy link
Contributor

cosmo0920 commented Jul 19, 2024 •
edited
Loading

Partially duplicated #8662.

@prince-chrismc
Copy link
Author

Seems like we needed the same feature at the same time 😂 let's hope we can get more traction!

@yangliuqing5470
Copy link

great job

@prince-chrismc
Copy link
Author

Found more related issues of others asking for this feature

#7010
https://laurierhodes.info/node/156 Published by Laurie Rhodes - Sun, 04/07/2024 - 10:58

@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Jul 29, 2024
@patrick-stephens
Copy link
Contributor

Do we need dependency updates for the various builds?
https://github.com/fluent/fluent-bit/tree/master/packaging/distros
https://github.com/fluent/fluent-bit/tree/master/dockerfiles

@prince-chrismc
Copy link
Author

I am sorry Patrick but I don't understand your question. Could you try rephrasing it differently?

This is windows only, so none of the distro packages are affected. For the windows docker, the source code for Kafka is the same that's already checked into the repo.

There are no changes to any dependencies or versionnin this pr.

@patrick-stephens
Copy link
Contributor

I am sorry Patrick but I don't understand your question. Could you try rephrasing it differently?

This is windows only, so none of the distro packages are affected. For the windows docker, the source code for Kafka is the same that's already checked into the repo.

There are no changes to any dependencies or versionnin this pr.

Ah my bad, just windows - I saw the cmake change and thought it was global.

@prince-chrismc
Copy link
Author

Phew! got scared I missed something important 🙈 Windows setting in CMake so it should not be global

@cosmo0920
Copy link
Contributor

However, AppVeyor complains that missing libcrypto.lib error:

LINK : warning LNK4044: unrecognized option '/lpthread'; ignored
LINK : fatal error LNK1104: cannot open file 'libcrypto.lib'
NMAKE : fatal error U1077: '"C:\Program Files\CMake\bin\cmake.exe"' : return code '0xffffffff'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\nmake.exe"' : return code '0x2'
Stop.
NMAKE : fatal error U1077: '"C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\bin\HostX64\x64\nmake.exe"' : return code '0x2'
Stop.
Test project C:/projects/fluent-bit-2e87g/build
Start 1: flb-it-pipe

@prince-chrismc
Copy link
Author

I am not sure how to move this forward, #5690 it seems that you already plan to remove Appveyor. I can't help but notice you duplicated this PR and did in fact disable appveyor in #8662 since it's not setup to handle windows.

It's missing OpenSSL from being installed in the pipeline

fluent-bit/appveyor.yml

Line 35 in a6aac45

install:
and this was addressed on GitHub Actions by adding VCPKG to handle the dependencies. This was support in the Windows Dockerfile,

fluent-bit/dockerfiles/Dockerfile.windows

Line 65 in a6aac45

RUN vcpkg install --recurse openssl --triplet x64-windows-static; `
. Is that what it would take? Appveyor is less accessible which makes it more difficult to work with.

As someone relying on the GitHub packages produced for Windows I would like to give my vote of confidence for making the switch.

@cosmo0920
Copy link
Contributor

It's missing OpenSSL from being installed in the pipeline

No. AppVeyor's Windows workers are already installed OpenSSL libraries.

ref: https://www.appveyor.com/docs/windows-images-software/

. Is that what it would take? Appveyor is less accessible which makes it more difficult to work with.

As someone relying on the GitHub packages produced for Windows I would like to give my vote of confidence for making the switch.

We didn't tried this. Because We had already used GitHub Actions to publish Windows packages. AppVeyor is no longer needed to create them.

@cosmo0920
Copy link
Contributor

Superserded by #8662.

@cosmo0920 cosmo0920 closed this Aug 9, 2024
@cosmo0920
Copy link
Contributor

And thanks for your vcpkg advice here. I applied your advice in #8692. Anyway thank you for your suggestions.

@prince-chrismc
Copy link
Author

Wonderful! I apologize I was not clear but I am glad this made it into the next release.
Looking forward to upgrading more regularly!

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujimotos fujimotos Awaiting requested review from fujimotos fujimotos is a code owner

@niedbalski niedbalski Awaiting requested review from niedbalski niedbalski is a code owner

@patrick-stephens patrick-stephens Awaiting requested review from patrick-stephens patrick-stephens is a code owner

@celalettin1286 celalettin1286 Awaiting requested review from celalettin1286 celalettin1286 is a code owner

Assignees
No one assigned
Labels
docs-required ok-package-test Run PR packaging tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@prince-chrismc @cosmo0920 @yangliuqing5470 @patrick-stephens